Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adapt time to fiori3 #801

Merged
merged 20 commits into from
Apr 24, 2020
Merged

feat: Adapt time to fiori3 #801

merged 20 commits into from
Apr 24, 2020

Conversation

JKMarkowski
Copy link
Contributor

@JKMarkowski JKMarkowski commented Mar 25, 2020

Related Issue

Closes #577

Description

There is whole time component refactored, right now it follows all of fiori3 specs. Also there are SAP theming parameters used.

Breaking Changes:

Refactored markup for time/time picker.
Removed Classes:

  • fd-time__control

New modes:

  • fd-time(--compact/--tablet/--collapsible)

New elements:

  • fd-time__col
  • fd-time__slider-label
  • fd-time__wrapper(--active/--meridian)
  • fd-time__item(--collapsed)
  • fd-time__unit

Screenshots

NOTE: If you've made any style changes, please provide appropriate screenshots (before and after) to help reviewers.

To see examples of which screenshots to include, go to Screenshot Examples.

Before:

image

TimePicker:
image
image

After:

Cozy:
image

Tablet:
image

Compact:
image

Time Picker:
image
image
image

@JKMarkowski JKMarkowski added 0.8.0 and removed 0.8.0 labels Mar 25, 2020
@JKMarkowski JKMarkowski added this to the Sprint 33 - London milestone Mar 25, 2020
@netlify
Copy link

netlify bot commented Mar 25, 2020

Deploy preview for fundamental-styles ready!

Built with commit 3541f5c

https://deploy-preview-801--fundamental-styles.netlify.app

@droshev droshev requested review from a team, estelaxu3 and Vanessa-Cusmich March 25, 2020 20:17
Copy link
Contributor

@stefanoScalzo stefanoScalzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2020-03-27 at 9 30 45 AM
Screen Shot 2020-03-27 at 9 31 50 AM
When normalizing and testing theming it doesn't seem quite right

@@ -7,31 +7,221 @@ $block: #{$fd-namespace}-time;
.#{$block} {
@include fd-reset();

width: 184px; // Quick fix to solve overflow issue
$fd-time-item-height: 2.875rem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 2 rem for cozy mode, it only gets bigger for smaller applications like tablet and phone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @stefan-sto compact mode has 2rem, default one has 2.875rem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JKMarkowski I think this is the wrong Stefano ;)

src/time.scss Outdated
height: $fd-time-item-compact-height;
overflow: hidden;
white-space: nowrap;
text-overflow: ellipsis;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to define a max-width for the overflow, ask the designers

Copy link
Contributor Author

@JKMarkowski JKMarkowski Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @stefan-sto I think max width should be 100% there. With text elipsis

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JKMarkowski wrong Stefano

@droshev droshev requested a review from trilodge March 27, 2020 22:06
@droshev
Copy link
Contributor

droshev commented Mar 27, 2020

@trilodge can you review the PR from a11y point of view?

@droshev
Copy link
Contributor

droshev commented Mar 29, 2020

we can review but merge once version 0.8.0 is release

src/time.scss Outdated
Comment on lines 24 to 26
display: flex;
flex-direction: column;
align-items: center;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use fd-flex-vertical-center() {flex-direction: column;}

src/time.scss Outdated
Comment on lines 53 to 56
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use @include fd-flex-center() {flex-direction: column;}

src/time.scss Outdated
Comment on lines 90 to 92
display: flex;
align-items: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@include fd-flex-center()

src/time.scss Outdated
Comment on lines 145 to 146
align-items: center;
justify-content: center;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this working with display: block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftovers :D

src/time.scss Outdated
Comment on lines 165 to 167
overflow: hidden;
white-space: nowrap;
text-overflow: ellipsis;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use @include fd-ellipsis()

@InnaAtanasova
Copy link
Contributor

I don't think it's allowed to have this situation

Screen Shot 2020-04-16 at 4 34 13 PM

@InnaAtanasova
Copy link
Contributor

Screen Shot 2020-04-16 at 4 37 50 PM

The width of the label should be the same as the sliders

@JKMarkowski
Copy link
Contributor Author

I don't think it's allowed to have this situation

Screen Shot 2020-04-16 at 4 34 13 PM

There is no way to handle issues like that in pure css

src/time.scss Outdated Show resolved Hide resolved
@InnaAtanasova
Copy link
Contributor

I don't think it's allowed to have this situation
Screen Shot 2020-04-16 at 4 34 13 PM

There is no way to handle issues like that in pure css

I was afraid you will say so :D We will fix it in NGX then

@InnaAtanasova
Copy link
Contributor

Looks great, just 1 tiny issue with RTL (the text is not centered)

Screen Shot 2020-04-20 at 5 01 59 PM

@JKMarkowski
Copy link
Contributor Author

@InnaAtanasova I just removed fd-form-label class usage, and put mixin instead.
@rengare Variables added

@InnaAtanasova InnaAtanasova self-requested a review April 23, 2020 15:13
@stefanoScalzo
Copy link
Contributor

Screen Shot 2020-04-23 at 11 54 13 AM
Screen Shot 2020-04-23 at 11 55 01 AM
Screen Shot 2020-04-23 at 11 56 21 AM
check the playground to normalized and check the theming.

@JKMarkowski
Copy link
Contributor Author

@stefanoScalzo Everything works fine, it's just about the uglify variables for tests
image

@JKMarkowski JKMarkowski merged commit a429870 into master Apr 24, 2020
@JKMarkowski JKMarkowski deleted the feat/time-fiori3 branch April 24, 2020 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Time to match Fiori 3 specs and the sap-theming
7 participants